Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lint: tighten linting #2771

Closed
wants to merge 5 commits into from
Closed

lint: tighten linting #2771

wants to merge 5 commits into from

Conversation

faddat
Copy link
Member

@faddat faddat commented Sep 17, 2022 •
edited
Loading

Closes: #1939

What is the purpose of the change

  • This PR will tighten linting across osmosis v12.
  • style
    • I changed instances of Asset and Basset to assetA and assetB since they didn't need to be exported, and the latter reads more easily.

Brief Changelog

  • enable revive linter, with below exclusions:
  exclude:
    - "var-naming"

Testing and Verifying

This PR should not change any APIs or the function of any code. If it does, it should be rejected.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? not yet
  • How is the feature or change documented? in changelog

@github-actions github-actions bot added C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/incentives C:x/lockup T:CI labels Sep 17, 2022
@faddat
Copy link
Member Author

faddat commented Sep 17, 2022 •
edited
Loading

@ValarDragon @p0mvn -- hey :D.

So, I removed the exclusion of stableswap from the unused linter, but there and still many things unused.

I'm thinking that I should tag them like:

//nolint:unused

I am also ignoring variable name change requests from revive and only making changes requested by captLocal.

@github-actions github-actions bot added C:x/epochs C:x/gamm Changes, features and bugs related to the gamm module. C:x/pool-incentives C:x/superfluid C:x/twap Changes to the twap module labels Sep 17, 2022
@faddat faddat marked this pull request as ready for review September 17, 2022 06:08
@faddat faddat requested a review from a team September 17, 2022 06:08
@faddat faddat changed the title lint: lint v12 and tighten linting lint: tighten linting Sep 18, 2022
@ValarDragon
Copy link
Member

ValarDragon commented Sep 19, 2022 •
edited
Loading

I found the golangci lint comments quite helpful!

I also don't agree with most of the changes as net beneficial. (Though I also don't think deliberation around them is particularly project moving either)

@faddat
Copy link
Member Author

faddat commented Sep 19, 2022

I agree about not deliberating, so if this doesn't seem real useful, I can just close this, OR I can revert some of the changes and comment them.

@ValarDragon
Copy link
Member

Want to PR just revive + your exclusions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/epochs C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/mint C:x/pool-incentives C:x/superfluid C:x/tokenfactory C:x/twap Changes to the twap module T:CI
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@faddat @ValarDragon